-
Notifications
You must be signed in to change notification settings - Fork 170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DPDK: Refactor rdma-core source build, allow arbitrary package build #3110
base: main
Are you sure you want to change the base?
Conversation
27fd8c7
to
cfe22b9
Compare
6bd48ba
to
dc03e6b
Compare
lisa/util/__init__.py
Outdated
@@ -47,7 +48,7 @@ | |||
# source - | |||
# https://github.com/django/django/blob/stable/1.3.x/django/core/validators.py#L45 | |||
__url_pattern = re.compile( | |||
r"^(?:http|ftp)s?://" # http:// or https:// | |||
r"^(?:http|s?ftp)s?://" # http:// or https:// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Update the comments to make it consistent with the patterns.
- Expand the patterns make it easy to read, and avoid invalid pattern like
sftps
,ftps
.(?:http|https|sftp|ftp)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either remove the comments or make it consistent with patterns.
lisa/util/__init__.py
Outdated
if raise_error: | ||
raise LisaException(failure_msg) | ||
else: | ||
log.info(failure_msg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be debug level by default, because it's not worth to raise an exception. It means it's ignorable error.
dc03e6b
to
bd77550
Compare
lisa/util/__init__.py
Outdated
def check_url( | ||
log: Logger, | ||
source_url: str, | ||
expected_package_name_pattern: Optional[Pattern[str]] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expected_path_pattern
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to emphasize it's just the filename portion, I'll make it expected_filename_pattern
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's a regexp pattern, you can just check the end parts. If it'a name like filename
, it limits usages to relative/full path validation.
bd77550
to
494ffb3
Compare
lisa/util/__init__.py
Outdated
source_url: str, | ||
expected_package_name_pattern: Optional[Pattern[str]] = None, | ||
allowed_protocols: Optional[List[str]] = None, | ||
expected_domains: Optional[List[str]] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expected_domain_pattern
. One pattern should be enough to validate all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started with just a regex match but it was a pain with annoying edge cases, so I started writing this version. My goal is making it easier for the caller, this way you can pass a couple of domains, a couple of protocols, and a filename pattern (or just a string) and restrict future users from downloading whatever tarball from wherever later with little room for error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
long story short I'm writing it for me so I don't mess it up next time I go to do this 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can test regexp at someplace like https://regex101.com. if it's string comparison, it will meet more problems actually. Like how to allow/disallow subdomains, or attack like "invalidmicrosoft.com"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, wish I hadn't even tried to handle this at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okedokey, moved to a pattern for domains,
1917e3c
to
82a4aa5
Compare
Fulfilling an ask to be able to test multiple rdma-core versions to allow development and regression tests. Setting rdma_core_source to either a git or tar.gz link allows you to build from source. For git either set rdma_core_ref or it will pick the latest version sorted tag.
4b47ec6
to
bb107e1
Compare
98c48e5
to
00c47a2
Compare
00c47a2
to
642a523
Compare
Fulfilling an ask to be able to test multiple rdma-core versions to allow development and regression tests. Refactors the rdma-core bits into their own own class.
Setting rdma_core_source to either a git or tar.gz link allows you to build from source. For git either set rdma_core_ref or it will pick the latest version sorted tag.